feat: Add alternative verification API and expand MessageDetails#14
feat: Add alternative verification API and expand MessageDetails#14yaronf merged 3 commits intoyaronf:mainfrom
Conversation
|
Hi @jvatic, thank you for the pull request. This is a reasonably large PR so it might take a bit of iteration. Here are some initial comments:
|
|
Thanks for the review @yaronf!
RE code deletion/test changes: My thinking was that it felt cleaner to for While code coverage remains about the same overall, there are a few error paths not covered when calling |
yaronf
left a comment
There was a problem hiding this comment.
Hi @jvatic, since the old VerifyRequest/Response APIs still remain public APIs of the package, I would rather have them tested explicitly, regardless of the internal implementation. So please duplicate the relevant tests.
Also, I made some very minor changes on the main branch, please make sure to merge them.
Once you're done with the tests, I'll be happy to merge the PR. Thanks!
- Add `Message` struct with a `Verify` method that can be used in place of `VerifyRequest` + `RequestDetails`, and `VerifyResponse` + `ResponseDetails`. - `MessageDetails` now contains created, expires, nonce, and tag params.
|
Hi @yaronf, thanks for the feedback! I've rebased on main and duplicated all the relevant tests (ones calling Please let me know if there's anything else I can clean up before merging. And thanks for being open to this PR :) (edit: I added the .editorconfig file due to some tests being dependent on whitespace that would otherwise be removed by gofmt.) |
|
Sorry, one more thing: please add a little (maybe one sentence) to the package overview in doc.go, to clarify where these new APIs are to be used. |
|
@yaronf sounds good! I've added a sentence to doc.go. |
yaronf
left a comment
There was a problem hiding this comment.
I thought I could fix that typo myself, but I can't. So please commit...
|
Oops, fixed! |
Thanks for creating & maintaining this library @yaronf!
This PR resolves a need we have at $employer to do some additional verification on the message artifacts (and removes having to build an intermediate
http.Requestobject since we're using this in an rpc context).Messagestruct with aVerifymethod that can be used in place ofVerifyRequest+RequestDetails, andVerifyResponse+ResponseDetails.MessageDetailsnow contains created, expires, nonce, and tag params.I'm happy to iterate on this a bit if there's any changes you'd like to see before accepting.